Avoiding redundant mounts by using /proc/mount info #1224

pshahzeb · 2017-05-05T22:25:04Z

Convert a short volume name to the long one. If required to a trip to vmdkops service
In every mount request, check if the volume is already mounted (info of /proc/mount)
refcount test which kills docker and verifies refcounting

Testing:
The refcount test added kills dockerd and verifies refcount (after refcount has been initialized) through logs.

Verified the same manually by forcefully disabling plugin and enabling it.

pshahzeb · 2017-05-05T22:27:55Z

For the repeating code between photon and vmdk driver: #1212

1. Convert a short volume name to the long one. If required to a trip to vmdkops service 2. In every mount request, check if the volume is already mounted (info of /proc/mount) 3. refcount test which kills docker and verifies refcounting Fixes #1220

msterin

Workflow and code look clean. A few comments inside, main ones being (1) please avoid cut-n-paste, this makes it hard to change/track code (2) no need to do volume.Get if Mount() already getting full volume name.
A few more small comments insides. All in all - good changed and fast delivery, thanks !

msterin · 2017-05-06T01:21:33Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -54,6 +55,8 @@ const (
 	fsTypeTag            = "Fs_Type"
 )

+var fullVolumeNameMap map[string]string // save full volume names to avoid trip to vmdkops service


it is 'mount id -> fullVOlumeName', right ? Or is is something else
Please add comment ("mountId->fullVolumeName map" or whatever it is)

I'd rename fullVolumeNameMap to mountIdMap.
Should this be member of VolumeDriver since it is a property of volumeDriver?

msterin · 2017-05-06T01:23:10Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -362,6 +367,19 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead

 // private function that does the job of mounting volume in conjunction with refcounting
 func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
+	// get volume metadata
+	status, err := d.GetVolume(r.Name)


you need GetVolume on short names only, so the check for IsFullVolumeName should happen before the GetVolume .

I think the algorithm is supposed to be as follows (for mount , in pseudocode)

if IsFullVolumeName(name) { name = r.Name } else { s.GetVolume () //get volume info name = FormFullVolumeName (r.Name, GetDatastore(result_of_volume_get) }

In mount, we need GetVolume always, since the volume meta data is used below for fstype and access type.

msterin · 2017-05-06T01:29:30Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -618,6 +633,16 @@ func (d *VolumeDriver) Unmount(r volume.UnmountRequest) volume.Response {
 		return volume.Response{Err: ""}
 	}

+	if plugin_utils.IsFullVolumeName(r.Name) != true {
+		fullVolName := fullVolumeNameMap[r.ID]


yes, this is the right way of doing things. mount() should be similar

msterin · 2017-05-06T01:30:00Z

vmdk_plugin/drivers/photon/photon_driver.go

+	if plugin_utils.IsFullVolumeName(r.Name) != true {
+		fullVolName := fullVolumeNameMap[r.ID]
+		if fullVolName == "" { // if ID not present in local map, do a get trip
+			status, _ := d.GetVolume(r.Name)


why no check for GetVolume failure ?

msterin · 2017-05-06T01:31:47Z

vmdk_plugin/drivers/photon/photon_driver.go

+			fullVolName = plugin_utils.GetFullVolumeName(r.Name, datastore)
+		}
+		r.Name = fullVolName
+	}


I suggest to wrap up L639->644 (with slight modification) in COnstructFullVolumeName (shortName string) string function returning full name (and error) . Otherwise cut-n-paste is on the way

msterin · 2017-05-06T01:33:38Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

@@ -458,6 +477,16 @@ func (d *VolumeDriver) Unmount(r volume.UnmountRequest) volume.Response {
 		return volume.Response{Err: ""}
 	}

+	if plugin_utils.IsFullVolumeName(r.Name) != true {
+		fullVolName := fullVolumeNameMap[r.ID]
+		if fullVolName == "" { // if ID not present in local map, do a get trip


I think I saw this code a few time already :-).
Please refactor to avoid cut-n-paste

msterin · 2017-05-06T01:35:21Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+)
+
+const (
+	linuxMountsFile = "/proc/mounts"


this was already defined somewhere. Did you fully move the definition here ? If not, please do

msterin · 2017-05-06T01:36:32Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+
+// GetMountInfo - return a map of mounted volumes and devices
+func GetMountInfo(mountRoot string) (map[string]string, error) {
+	volumeMap := make(map[string]string)


Please add a comment of what is mapped to what (even if it's an old code). Something like // maps volume->fullMountPath or whatever it is doing

msterin · 2017-05-06T01:38:33Z

vmdk_plugin/utils/refcount/refcnt.go

-				r.Incr(mount.Name)
-				log.Infof("  name=%v (driver=%s source=%s) (%v)",
+			// check if the mount location belongs to vmdk plugin
+			if matchNameforVMDK(mount.Source) != true {


I think the comment is better than the function name. It would be nice to name it isVMDKMount() or something like that. matchNameforVMDK is confusing

msterin · 2017-05-06T01:39:14Z

vmdk_plugin/utils/refcount/refcnt.go

+			volname := mount.Name
+			if plugin_utils.IsFullVolumeName(volname) != true {
+				if datastore == "" {
+					volumeMeta, _ := d.GetVolume(volname)


and I certainly saw this code already. Please refactor to avoid cut-n-paste.

pdhamdhere

Looks good. Please check comments inline.

pdhamdhere · 2017-05-06T02:49:32Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -54,6 +55,8 @@ const (
 	fsTypeTag            = "Fs_Type"
 )

+var fullVolumeNameMap map[string]string // save full volume names to avoid trip to vmdkops service


I'd rename fullVolumeNameMap to mountIdMap.
Should this be member of VolumeDriver since it is a property of volumeDriver?

pdhamdhere · 2017-05-06T03:11:34Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -362,6 +367,19 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead

 // private function that does the job of mounting volume in conjunction with refcounting
 func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
+	// get volume metadata
+	status, err := d.GetVolume(r.Name)


Too many tiny helper functions. How about;

getFullVolumeName(name) { if (strings.ContainsAny(name, "@")) { return name; } volMeta, err := d.GetVolume(name) if err != nil return strings.Join(name, "@", volumeMeta["datastore"]) else ???? return err? }

pdhamdhere · 2017-05-06T03:20:50Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -618,6 +633,16 @@ func (d *VolumeDriver) Unmount(r volume.UnmountRequest) volume.Response {
 		return volume.Response{Err: ""}
 	}

+	if plugin_utils.IsFullVolumeName(r.Name) != true {


This check is unnecessary. fullVolumeNameMap[r.ID] if exists will have fullVolName

pdhamdhere · 2017-05-06T03:29:35Z

vmdk_plugin/drivers/photon/photon_driver.go

+	if plugin_utils.IsFullVolumeName(r.Name) != true {
+		fullVolName := fullVolumeNameMap[r.ID]
+		if fullVolName == "" { // if ID not present in local map, do a get trip
+			status, _ := d.GetVolume(r.Name)


status => volMeta ?

pdhamdhere · 2017-05-06T03:32:15Z

vmdk_plugin/drivers/photon/photon_driver.go

+		}
+		r.Name = fullVolName
+	}
+


Dont you have to delete mountID => volName entry from map?

govint · 2017-05-07T13:16:52Z

misc/scripts/refcnt_test.sh

+    echo "Checking recovery through docker kill"
+    # kill docker daemon forcefully
+    pkill -9 dockerd
+    until pids=$(pidof dockerd)


How does this impact the refcount module. Unless the plugin is restarted, the refcount module hasn't lost it's refcounts to do a recovery.

govint · 2017-05-07T13:25:50Z

misc/scripts/refcnt_test.sh

    sync  # give log the time to flush
    wait_for check_recovery_record $timeout
    if [ "$?" -ne 0 ] ; then
-        echo PLUGIN RESTART TEST FAILED. Did not find proper recovery record
+        echo DOCKER RESTART TEST FAILED. Did not find proper recovery record


Not sure this change is needed. The test is to see if the plugin restarts then the refcounts are restored. If the plugin restart isn't possible then this test should be made manual to execute with a standalone binary and deprecated from automated test run.

This test will in turn test plugin restart (docker restart). Plugin restart through docker restart tests the docker api consumption, timeout and handling of parallel mounts/unmounts till refcounting is successful. Not sure why not have this as automated test?

pshahzeb · 2017-05-08T21:19:18Z

vmdk_plugin/utils/refcount/refcnt.go

+			}
+			volname := mount.Name
+			// gets hit once to retrieve the default datastore. datastoreName is reused henceforth
+			if datastoreName == "" {


this is done once, but needs to be done inside the loop since we need a volume name to get the datastore.

pshahzeb · 2017-05-08T21:20:10Z

@msterin @pdhamdhere I have addressed your comments in the latest commit. Please take a look.

pdhamdhere · 2017-05-08T21:44:37Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -362,6 +365,19 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead

 // private function that does the job of mounting volume in conjunction with refcounting
 func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
+	// get volume metadata
+	volumeMeta, err := d.GetVolume(r.Name)


If plugin_utils.AlreadyMounted(r.Name, d.mountRoot) then you can skip d.GetVolume(r.Name) right?

saving VolumeMeta as part of retrieving full volume name. If found, avoiding next get trip.

pdhamdhere · 2017-05-09T00:48:48Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+	// get volume metadata if required
+	if volumeMeta == nil {
+		if volumeMeta, err = d.ops.Get(r.Name); err != nil {
+			return volume.Response{Err: err.Error()}


Dont you have to decrRefCount?

pdhamdhere · 2017-05-09T00:57:57Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+}
+
+// GetFullNameAndMeta - return a qualified volume and metadata(if a get trip was made)
+func GetFullNameAndMeta(name string, datastoreName string, d drivers.VolumeDriver) (string, map[string]interface{}, error) {


I am not go expert and don't know what is recommended practice here. It definitely seems inconsistent with rest of the code which generally returns "retValue, err" tuple. Should we change it to GetVolumeInfo to return dictionary "volInfo, err" tuple where volInfo.fullName and volInfo.metadata has required details.

I also suggested GetVolumeInfo. We can leave it with 3 ret vals but need to accurately document IMO

Yes. wrapping this in volInfo to contain. It will make code cleaner.

pdhamdhere · 2017-05-09T00:59:22Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -360,8 +363,21 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead
 	return mountpoint, fs.MountWithID(mountpoint, fstype, id, isReadOnly)
 }

+//VolumesInRefMap - get list of volumes from refmap


nit: space between // and VolumesInRefMap

another nit - please mention in the comments that these are full names volume@datastore format

pdhamdhere · 2017-05-09T01:07:40Z

vmdk_plugin/drivers/photon/photon_driver.go

 	if !exists {
 		fstype = fs.FstypeDefault
 	}

 	skipAttach := false
 	// If the volume is already attached to the VM, skip the attach.
-	if state, stateExists := status["State"]; stateExists {
+	if state, stateExists := volumeMeta["State"]; stateExists {
 		if strings.Compare(state.(string), "DETACHED") != 0 {


This is not your change but please file an issue and let's revisit.
Generally we don't make decision based on data in KV file and definitely not inside guest. Disk may be attached to some other VM. If neither refCnt nor mount map suggest disk is attached to this VM then we must issue attach to ESX. If it is already attached, it is a no-op from ESX perspective anyway.

msterin

Looks good. A bunch of small nits (comments and spaces mainly) and only 1 comment - I think @ split/join should be wrapped in a simple interface for volumes names (volname.join, volname.split, etc). And then it is good to go IMO

msterin · 2017-05-09T05:38:25Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -360,8 +363,21 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead
 	return mountpoint, fs.MountWithID(mountpoint, fstype, id, isReadOnly)
 }

+//VolumesInRefMap - get list of volumes from refmap


another nit - please mention in the comments that these are full names volume@datastore format

msterin · 2017-05-09T05:40:23Z

vmdk_plugin/drivers/photon/photon_driver.go

-	}
-
-	r.Name, err = plugin_utils.GetFullVolumeName(r.Name, volumeMeta["datastore"].(string), d)
+	volname, volumeMeta, err := plugin_utils.GetFullNameAndMeta(r.Name, "", d)


nit: the name is accurate but confusing IMO. I'd simply call it GetVolInfo() and added comment
//returns fullName. volumeMeta map and err if any

msterin · 2017-05-09T05:42:12Z

vmdk_plugin/drivers/photon/photon_driver.go

 	if err != nil {
 		log.Errorf("Unable to get full name for volume %s. err:%v", r.Name, err)
 		return volume.Response{Err: err.Error()}
 	}
+	r.Name = volname
+	d.mountIDtoName[r.ID] = volname


nit - I'd use r.Name here, not volname (just like everywhere till the rest of the function)

msterin · 2017-05-09T05:44:17Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

 	if err != nil {
 		log.Errorf("Unable to get full name for volume %s. err:%v", r.Name, err)
 		return volume.Response{Err: err.Error()}
 	}
+	r.Name = volname
+	d.mountIDtoName[r.ID] = volname


same nit - (use r.Name not volname., as everyone till the end of this function)

msterin · 2017-05-09T05:45:07Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

 	}
-	return volumeMeta["datastore"].(string), nil
+	return volumeMeta["datastore"].(string), volumeMeta, nil


nit - in other places volumeMeta is indexed by a named constant, not literal. Why sudden change ?

msterin · 2017-05-09T05:47:21Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

 }

-// GetFullVolumeName - append datastore to the volume name
-func GetFullVolumeName(name string, datastoreName string, d drivers.VolumeDriver) (string, error) {
+// GetNameFromRefmap - get names from refmap


nit: please add comment. Something like "finds an entry for volume. Entry can be in vol@ds format, Volums is passed in as a short name, with no '@ds' (btw 0 is this what you intended to do ? )

msterin · 2017-05-09T05:48:15Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+		}
+		count++
+		fullname = name
+	}


so this one scans the full list regardess. Generally not nice, but I guess these lists are short...

msterin · 2017-05-09T05:49:07Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+}
+
+// GetFullNameAndMeta - return a qualified volume and metadata(if a get trip was made)
+func GetFullNameAndMeta(name string, datastoreName string, d drivers.VolumeDriver) (string, map[string]interface{}, error) {


I also suggested GetVolumeInfo. We can leave it with 3 ret vals but need to accurately document IMO

msterin · 2017-05-09T05:50:17Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

 	}
 	if datastoreName != "" {
-		return strings.Join([]string{name, datastoreName}, "@"), nil
+		return strings.Join([]string{name, datastoreName}, "@"), nil, nil


I do not like the joins and splits sprinkled over the code.
Can it we wrapped in a couple of fucntions (similar to python os.path.join and os.path,basename) ?

msterin · 2017-05-09T05:51:08Z

vmdk_plugin/utils/refcount/refcnt.go

@@ -264,6 +264,19 @@ func (r *RefCountsMap) GetCount(vol string) uint {
 	return rc.count
 }

+//GetVolumeNames - return volume names of entries in refmap


space after //.
Also - full names ? Where do we use a full list of volnames ?

Yes. Just one additional optimization to get full volume name from entries in refmap(if there no multiple entries for given volume name) and avoid a trip to esx

// GetVolumeNames - return fully qualified volume names from refMap

govint · 2017-05-09T18:15:31Z

vmdk_plugin/utils/refcount/refcnt.go

+	}
+
+	return volumeList
+}


Possibly add a test that creates and mounts volumes with the same name on different datastores.

Sure. did you mean "list can contain volumes with same name on different datastores?"

govint · 2017-05-09T18:46:19Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+}
+
+// GetNameFromRefmap - get names from refmap
+func GetNameFromRefmap(volName string, d drivers.VolumeDriver) string {


Nit - could make this a little descriptive comment especially how volumes with the same name on different datastores are handled.

Plus what happens in that case and this function returns an empty string. Its not wrong to have volumes with the same name on different datastores.

General - Pls. add a descriptive doc update of this change to any existing refcounting doc,

govint · 2017-05-09T18:57:22Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+	// find full volume names using refmap if possible
+	if fullVolumeName := GetNameFromRefmap(name, d); fullVolumeName != "" {
+		return fullVolumeName, nil, nil
+	}


Unlike what the name suggests "GetFullNameAndMeta()" most of the returns here return NULL for the meta. Is that intended? If only one condition is going to return the metadata for a volume then suggest that this function just return the full name or even merge it with the calling function.

Should never mix multiple unrelated actions inside a single function. - doesn't need to return the meta-data for a volume GetVolume() should be used for that.

Yes renaming it. The idea was to return additional information along with full name so as to avoid trips to esx

govint · 2017-05-09T18:59:27Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+// GetDatastore - get datastore from volume metadata
+// Note "datastore" key is defined in vmdkops service
+func GetDatastore(name string, d drivers.VolumeDriver) (string, map[string]interface{}, error) {
+	volumeMeta, err := d.GetVolume(name)


Function is returning values that are not seemingly related to the name of the function - GetDatastore(). Preferably remove this and let the calling function just call GetVolume() and pick the datastore entry from the return meta-data.

If this function is a must-have then let it return just the datastore name. I mean we are returning the datastore name and the meta-data which also has the same datastore name in it.

refactored. This function is no more needed. did the getvolume in the calling function.

…etvolume failure

pshahzeb · 2017-05-09T19:06:49Z

vmdk_plugin/utils/refcount/refcnt.go

-			volname := mount.Name
-			// gets hit once to retrieve the default datastore. datastoreName is reused henceforth
-			if datastoreName == "" {
-				datastoreName, _, err = plugin_utils.GetDatastore(volname, d)


this isn't needed anymore. returned datastore as part of volumeInfo. captured here to be reused further (and avoid trip to esx)

pdhamdhere

LGTM

pdhamdhere · 2017-05-09T19:36:02Z

vmdk_plugin/utils/plugin_utils/plugin_utils.go

+}
+
+// GetVolumeInfo - return VolumeInfo which a qualified volume name,
+// datastore name volume metadata if retrieved


How about changing comment to following?

return VolumeInfo with a qualified volume name.
Optionally returns datastore and volume metadata if retrieved from ESX.
If Volume Metadata is nil then caller can use getVolume()

pdhamdhere · 2017-05-09T19:38:08Z

vmdk_plugin/utils/refcount/refcnt.go

@@ -264,6 +264,19 @@ func (r *RefCountsMap) GetCount(vol string) uint {
 	return rc.count
 }

+//GetVolumeNames - return volume names of entries in refmap


// GetVolumeNames - return fully qualified volume names from refMap

pshahzeb requested review from msterin and pdhamdhere May 5, 2017 22:25

vmwclabot added the cla-not-required label May 5, 2017

pshahzeb requested a review from shaominchen May 5, 2017 22:39

pshahzeb force-pushed the issue_1220.pshahzeb branch from 943ae20 to be205c1 Compare May 5, 2017 23:33

msterin reviewed May 6, 2017

View reviewed changes

pdhamdhere reviewed May 6, 2017

View reviewed changes

govint reviewed May 7, 2017

View reviewed changes

Addressing comments from Mark and Prashant

e88907c

pshahzeb force-pushed the issue_1220.pshahzeb branch from c89cae0 to e88907c Compare May 8, 2017 21:16

pshahzeb commented May 8, 2017

View reviewed changes

pdhamdhere reviewed May 8, 2017

View reviewed changes

Use entry from refmap and avoid get trip to esx

62782d9

pdhamdhere reviewed May 9, 2017

View reviewed changes

msterin reviewed May 9, 2017

View reviewed changes

govint reviewed May 9, 2017

View reviewed changes

Wrapper struct for GetVolumeInfo util, cosmetics and refcnt decr on g…

51468d6

…etvolume failure

pshahzeb commented May 9, 2017

View reviewed changes

pdhamdhere approved these changes May 9, 2017

View reviewed changes

minor comments

f7e3360

msterin approved these changes May 9, 2017

View reviewed changes

pshahzeb merged commit 4eca684 into master May 9, 2017

shuklanirdesh82 deleted the issue_1220.pshahzeb branch May 13, 2017 13:47

Avoiding redundant mounts by using /proc/mount info #1224

Avoiding redundant mounts by using /proc/mount info #1224

Conversation

pshahzeb commented May 5, 2017

pshahzeb commented May 5, 2017

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshahzeb commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 9, 2017 • edited Loading

Choose a reason for hiding this comment

govint May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govint May 9, 2017 •

edited

Loading

govint May 9, 2017 •

edited

Loading

govint May 9, 2017 •

edited

Loading